Skip to content

hir_owner_parent optimized to inlined call for non-incremental build #147387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

hir_owner_parent optimized to inlined call for non-incremental build #147387

wants to merge 1 commit into from
+29 −15

Conversation

azhogin
Copy link
Contributor

@azhogin azhogin commented Oct 5, 2025 •
edited
Loading

Continuation of #146880 and #147232.
'hir_owner_parent' query renamed 'hir_owner_parent_q'. hir_owner_parent inlined function added to optimize performance in case of non-incremental build.

'hir_owner_parent' query has low normalized average execution time (163ns) and good cache_hits (5773) according Daria's processed statistics. 'source_span', for comparison, has avg_ns_norm = 66ns and cache_hits = 11361.

Optimization may be profitable for queries with low normalized average execution time (to replace cache lookup into inlined call) and be significant with good cache_hits.

Query cache_hits min_ns max_ns avg_ns_norm
source_span 11361 18 2991 66
hir_owner_parent 5773 52 1773 163
is_doc_hidden 3134 47 1111 285
lookup_deprecation_entry 13905 36 6208 287
object_lifetime_default 5840 63 4688 290
upvars_mentioned 2575 75 7722 322
intrinsic_raw 21235 73 3453 367

Draft PR to measure performance changes.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 5, 2025
@Kobzol
Copy link
Member

Kobzol commented Oct 6, 2025

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rust-bors

This comment has been minimized.

Sign in to view
rust-bors bot added a commit that referenced this pull request Oct 6, 2025
@rust-bors
Auto merge of #147387 - azhogin:azhogin/hir_owner_parent_opt, r=<try>
f98c273 
hir_owner_parent optimized to inlined call for non-incremental build
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 6, 2025
@rust-bors
Copy link

rust-bors bot commented Oct 6, 2025

☀️ Try build successful (CI)
Build commit: f98c273 (f98c27362fe04fb22b3c3db681405edf9f642465, parent: 828c2a9afccf3b3ff8133368cfbc8bfe526aaa4d)

@rust-timer

This comment has been minimized.

Sign in to view
@azhogin
Copy link
Contributor Author

azhogin commented Oct 6, 2025

r? @petrochenkov

@rustbot
Copy link
Collaborator

rustbot commented Oct 6, 2025

Requested reviewer is already assigned to this pull request.

Please choose another assignee.

@azhogin
Copy link
Contributor Author

azhogin commented Oct 6, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 6, 2025
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f98c273): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 0.0% [0.0%, 0.0%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.5% [-1.1%, -0.1%] 10
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -1.0%, secondary 0.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.3% [2.2%, 2.3%] 2
Improvements ✅
(primary)		 -1.0% [-1.0%, -1.0%] 1
Improvements ✅
(secondary)		 -1.9% [-2.3%, -1.6%] 2
All ❌✅ (primary) -1.0% [-1.0%, -1.0%] 1

Cycles

Results (secondary 1.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 5.3% [5.3%, 5.3%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.7% [-2.7%, -2.7%] 1
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 469.991s -> 471.779s (0.38%)
Artifact size: 388.38 MiB -> 388.40 MiB (0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 6, 2025
@cjgillot
Copy link
Contributor

cjgillot commented Oct 6, 2025

You posted several PRs with this same idea. Instead of trying all queries, is there a way to address this structurally? I mean: change a macro in the query system to do this automatically?

@petrochenkov
Copy link
Contributor

I mean: change a macro in the query system to do this automatically?

That was the plan if the optimization worked.
It didn't work previously, so this PR and #147388 were going to be the last two attempts, but it seems like in this case it may actually work.

@petrochenkov
Copy link
Contributor

@azhogin
Could you write a bit of background in the PR descriptions? It's not just me who is reading them.
Mention that it's a continuation of #146880 and #147232, what is the idea behind the optimization, how is this query different from queries from the previous attempts, etc.

petrochenkov
petrochenkov reviewed Oct 6, 2025
View reviewed changes
@petrochenkov
Copy link
Contributor

but it seems like in this case it may actually work.

I wonder why it works though.
The logic inside the provider function doesn't look obviously cheaper than a query cache lookup.
And if it's actually cheaper, then why didn't the previous PRs work. Perhaps the hir_owner_parent query is just much hotter?

change a macro in the query system to do this automatically?

A drawback in this is that you'll probably need a function pointer, and it won't be possible to replace a query call with an inlined cheap function call.
In any case, adding a macro probably doesn't make sense until there are at least several queries on which this optimization works.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 6, 2025
@azhogin azhogin force-pushed the azhogin/hir_owner_parent_opt branch from acb23dc to 5aae413 Compare October 7, 2025 12:01
petrochenkov
petrochenkov approved these changes Oct 7, 2025
View reviewed changes
compiler/rustc_middle/src/hir/mod.rs
}
#[inline]
pub fn hir_owner_parent(self, owner_id: OwnerId) -> HirId {
if self.dep_graph.is_fully_enabled() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function needs a comment explaining this unusual setup.

@petrochenkov
Copy link
Contributor

r? @cjgillot to confirm that tcx.dep_graph.is_fully_enabled() is the right condition, and maybe answering to the comments above.

@rustbot rustbot assigned cjgillot and unassigned petrochenkov Oct 7, 2025
@azhogin
Copy link
Contributor Author

azhogin commented Oct 7, 2025 •
edited
Loading

@azhogin Could you write a bit of background in the PR descriptions?

Description improved. I have added top of processed query info table for minimal 'avg_ns_norm' in the description.

@azhogin azhogin force-pushed the azhogin/hir_owner_parent_opt branch from 5aae413 to 7939302 Compare October 7, 2025 14:52
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@azhogin azhogin force-pushed the azhogin/hir_owner_parent_opt branch from 7939302 to a5e8e86 Compare October 7, 2025 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov approved these changes

Assignees

@cjgillot cjgillot

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@azhogin @Kobzol @rust-timer @rustbot @cjgillot @petrochenkov @rust-log-analyzer